Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces optional custom property support at signup via Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as SDK/Auth
participant Backend as Backend Auth
participant RuleEngine as Rule Engine
participant UserDB as User DB
Client->>SDK: signInWithMagicCode({ email, code, extraFields })
SDK->>Backend: POST /runtime/auth/verify_magic_code<br/>(email, code, extra-fields)
Backend->>Backend: Fetch existing user by email
Backend->>RuleEngine: Check $users.create rule<br/>(if new user)
alt User creates
RuleEngine-->>Backend: Permission check result
Backend->>Backend: validate-extra-fields!
Backend->>RuleEngine: Evaluate create rule<br/>with data context
RuleEngine-->>Backend: Permission granted/denied
end
alt Permission granted OR existing user
Backend->>UserDB: Create or update user with extra-fields
Backend->>UserDB: Consume magic code
Backend-->>SDK: { user, created: true/false, refresh_token }
else Permission denied
Backend-->>SDK: HTTP 400 validation error
end
SDK-->>Client: { user, created }
sequenceDiagram
participant Client
participant SDK as SDK/Reactor
participant Storage as Local Storage
participant Backend as OAuth Backend
participant Exchange as Token Exchange
Client->>SDK: createAuthorizationURL<br/>({ clientName, redirectURL, extraFields })
SDK->>Storage: Store extraFields under oauthExtraFieldsKey
SDK-->>Client: OAuth authorization URL
Client->>Backend: Initiate OAuth flow
Client->>SDK: _oauthLoginInit (redirect callback)
SDK->>Storage: Retrieve and delete extraFields
SDK->>Exchange: exchangeCodeForToken<br/>({ code, extraFields })
Exchange->>Backend: POST /runtime/auth/oauth/callback<br/>(with extra_fields)
Backend-->>Exchange: { user, created, refresh_token }
Exchange-->>SDK: { user, created }
SDK-->>Client: User signed in with custom properties
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View Vercel preview at instant-www-js-extra-fields-jsv.vercel.app. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/instant/runtime/routes.clj (1)
586-600:⚠️ Potential issue | 🟠 MajorValidate
extra_fieldsbefore consuming the OAuth code.
app-oauth-code-model/consume!runs beforevalidate-extra-fields!. If the request carries an unknown/system field, the call fails after the one-time OAuth code is already spent, so the user cannot retry with corrected input.Suggested fix
(let [app-id (ex/get-some-param! req (param-paths :app_id) uuid-util/coerce) code (ex/get-some-param! req (param-paths :code) uuid-util/coerce) code-verifier (some #(get-in req %) (param-paths :code_verifier)) extra-fields (get-in req [:body :extra_fields]) + _ (app-user-model/validate-extra-fields! app-id extra-fields) oauth-code (app-oauth-code-model/consume! {:code code :app-id app-id :verifier code-verifier}) @@ - _ (assert (= app-id app_id) (str "(= " app-id " " app_id ")")) - _ (app-user-model/validate-extra-fields! app-id extra-fields) + _ (assert (= app-id app_id) (str "(= " app-id " " app_id ")"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/runtime/routes.clj` around lines 586 - 600, Move validation of request extra_fields to run before consuming the one-time OAuth code: retrieve extra-fields from req (extra-fields), call app-user-model/validate-extra-fields! with app-id and extra-fields first, then call app-oauth-code-model/consume!; keep the existing origin header check and the subsequent assertions the same but ensure app-user-model/validate-extra-fields! runs before app-oauth-code-model/consume! to avoid spending the code on invalid input.
🧹 Nitpick comments (3)
client/packages/create-instant-app/template/rules/cursor-rules.md (1)
286-308: Inconsistent transaction reference in code example.The code example on line 302 uses
tx.settings[id()], but the convention shown elsewhere in this file (line 243) uses a fully-qualified reference likedb.tx.todos[id()]. For consistency and clarity, the example should show wheretxcomes from or use the fulldb.txreference.📝 Suggested fix for consistency
// Scaffold data for new users if (created) { db.transact([ - tx.settings[id()] + db.tx.settings[id()] .update({ theme: 'light', notifications: true }) .link({ user: user.id }), ]); }Otherwise, the documentation accurately reflects the new
extraFieldsAPI and thecreatedflag, and the code examples effectively demonstrate the key use cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/create-instant-app/template/rules/cursor-rules.md` around lines 286 - 308, The example uses an undefined local `tx` (tx.settings[id()]) causing inconsistency; update the snippet to reference the full transaction helper `db.tx` (e.g., use db.tx.settings[id()]) or explicitly show how `tx` is defined before use; ensure the example uses the same convention as other examples with signInWithMagicCode/extraFields and the created flag so the db.transact call references db.tx.settings[id()] (or introduces a prior const tx = db.tx) for clarity.client/packages/core/vitest.config.ts (1)
4-5: LGTM with a minor observation on edge case handling.The nullish coalescing (
?? 0) handlesundefined, but ifDEV_SLOTis set to a non-numeric string (e.g.,DEV_SLOT=abc),localPortwill beNaN. Given the context ine2e.ts, this is safe sinceNaNis falsy and tests would fall back to production URLs—but this silent fallback might mask configuration errors.If explicit validation is preferred, consider:
🔧 Optional: Add validation for DEV_SLOT
-const devSlot = Number(process.env.DEV_SLOT ?? 0); -const localPort = process.env.CI ? 0 : 8888 + devSlot * 1000; +const devSlot = Number(process.env.DEV_SLOT ?? 0); +if (Number.isNaN(devSlot)) { + throw new Error(`Invalid DEV_SLOT value: ${process.env.DEV_SLOT}`); +} +const localPort = process.env.CI ? 0 : 8888 + devSlot * 1000;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/core/vitest.config.ts` around lines 4 - 5, The code uses Number(process.env.DEV_SLOT ?? 0) which yields NaN for non-numeric DEV_SLOT values; update the logic in vitest.config.ts around devSlot and localPort to validate DEV_SLOT explicitly: read the raw string from process.env.DEV_SLOT, attempt to parse it (e.g., parseInt/Number), check isFinite/Number.isNaN, and if invalid fall back to 0 (or optionally throw) and compute localPort from the validated devSlot so stray non-numeric environment values no longer produce NaN silently.client/packages/core/__tests__/src/auth-extra-fields.e2e.test.ts (1)
57-70: Cover the “ignoreextraFieldsfor existing users” contract.This test proves
created=false, but it would still pass if a later sign-in overwrote the existing$usersrow. Since that behavior is a core contract in this PR, re-sign in with differentextraFieldsand assert the stored values stay unchanged.💡 Suggested test expansion
authTest( 'returning user gets created=false', async ({ db, appId, adminToken }) => { const email = `returning-${Date.now()}@test.com`; // First sign in -- creates user const code1 = await generateMagicCode(appId, adminToken, email); - const res1 = await db.auth.signInWithMagicCode({ email, code: code1 }); + const res1 = await db.auth.signInWithMagicCode({ + email, + code: code1, + extraFields: { username: 'first_user', displayName: 'First User' }, + }); expect(res1.created).toBe(true); // Second sign in -- existing user const code2 = await generateMagicCode(appId, adminToken, email); - const res2 = await db.auth.signInWithMagicCode({ email, code: code2 }); + const res2 = await db.auth.signInWithMagicCode({ + email, + code: code2, + extraFields: { username: 'second_user', displayName: 'Second User' }, + }); expect(res2.created).toBe(false); + + const { data } = await db.queryOnce({ $users: {} }); + const user = data.$users.find((u: any) => u.email === email); + expect(user!.username).toBe('first_user'); + expect(user!.displayName).toBe('First User'); }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/core/__tests__/src/auth-extra-fields.e2e.test.ts` around lines 57 - 70, The test currently only asserts created=false but doesn't verify that extraFields were not overwritten; update the test in auth-extra-fields.e2e.test.ts to capture the stored user after the first sign-in (use the returned user id or query the $users row) then perform the second sign-in with different extraFields via generateMagicCode and db.auth.signInWithMagicCode, and finally re-fetch the same $users row and assert its extraFields still equal the original values (and that res2.created is false) to ensure existing users are not updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/packages/core/src/Reactor.js`:
- Around line 2270-2276: The createAuthorizationURL method currently only writes
OAUTH_EXTRA_FIELDS_KEY to sessionStorage when extraFields is provided, leaving
stale data from prior attempts; update createAuthorizationURL so that when
sessionStorage is available but extraFields is falsy it explicitly removes
OAUTH_EXTRA_FIELDS_KEY (or sets it to null) from sessionStorage to prevent
_oauthLoginInit from reusing stale fields; refer to createAuthorizationURL,
OAUTH_EXTRA_FIELDS_KEY and _oauthLoginInit when making this change.
In `@client/packages/create-instant-app/template/rules/AGENTS.md`:
- Around line 296-299: The example uses an undefined variable `tx` causing a
ReferenceError; update the scaffold to call the transaction builder off `db`
(use `db.tx` instead of `tx`) when constructing the operations such as
`tx.settings[id()].update(...)` and `db.transact(...)` — ensure the snippet
consistently references `db.tx` (and `db.transact`) so symbols like
`db.transact`, `db.tx`, and `settings[id()]` are used correctly and no undefined
`tx` remains.
In `@client/packages/create-instant-app/template/rules/windsurf-rules.md`:
- Around line 286-308: The transaction example uses an undefined tx variable;
update the transaction to use the DB transaction namespace by changing the call
in the created branch to use db.tx.settings (i.e., replace tx.settings[id()]
with db.tx.settings[id()]) so the scaffold uses db.transact([...
db.tx.settings[id()].update(...).link(...) ...]) alongside the existing
db.auth.signInWithMagicCode, extraFields, created, and db.transact usage.
In `@client/sandbox/react-nextjs/pages/play/oauth-extra-fields-google.tsx`:
- Around line 101-130: Move the call to db.auth.createAuthorizationURL out of
the component render and into the click handler for the "Google (Redirect)"
control: remove redirectLoginURL from render (and do not call
createAuthorizationURL during SSR), add an onClick handler for the anchor/button
that constructs the URL using db.auth.createAuthorizationURL({ clientName:
'google-web', redirectURL: window.location.href, extraFields: displayName ? {
displayName } : undefined }) only when running in the browser, then navigate
(e.g. window.location.href = url); this avoids accessing window during SSR and
prevents createAuthorizationURL's sessionStorage side effects from running on
render/Strict Mode.
In `@client/www/lib/intern/instant-rules.md`:
- Around line 296-301: The example uses an undefined bare symbol tx inside
db.transact; update the snippet to use the database object's transaction helper
(replace tx with db.tx) so it matches other examples (e.g., clientDb.tx) and is
import/usage-consistent; modify the call to db.transact([...
db.tx.settings[id()].update({ theme: 'light', notifications: true }).link({
user: user.id }) ]) accordingly so the example compiles and follows the same
pattern.
In `@client/www/pages/docs/backend.md`:
- Around line 403-408: The snippet incorrectly destructures { user, created }
from verifyMagicCode as if using the client SDK; in the Admin SDK
verifyMagicCode returns the user object (with user.created), so change the call
to capture the user directly (e.g., const user = await
db.auth.verifyMagicCode(...)) and then read token from user.refresh_token and
created from user.created; update any subsequent uses (token and created) to
reference the user object.
In `@client/www/pages/docs/users.md`:
- Around line 206-216: The docs example calls db.auth.createAuthorizationURL
during render which writes extraFields to sessionStorage; change the example to
call db.auth.createAuthorizationURL (or the wrapper createAuthorizationURL)
inside a user interaction handler (e.g., an onClick handler) immediately before
performing navigation so extraFields are saved at the correct time; locate the
usage of createAuthorizationURL in the docs example and move its invocation into
the click handler and then navigate to the returned URL, rather than computing
it during render.
---
Outside diff comments:
In `@server/src/instant/runtime/routes.clj`:
- Around line 586-600: Move validation of request extra_fields to run before
consuming the one-time OAuth code: retrieve extra-fields from req
(extra-fields), call app-user-model/validate-extra-fields! with app-id and
extra-fields first, then call app-oauth-code-model/consume!; keep the existing
origin header check and the subsequent assertions the same but ensure
app-user-model/validate-extra-fields! runs before app-oauth-code-model/consume!
to avoid spending the code on invalid input.
---
Nitpick comments:
In `@client/packages/core/__tests__/src/auth-extra-fields.e2e.test.ts`:
- Around line 57-70: The test currently only asserts created=false but doesn't
verify that extraFields were not overwritten; update the test in
auth-extra-fields.e2e.test.ts to capture the stored user after the first sign-in
(use the returned user id or query the $users row) then perform the second
sign-in with different extraFields via generateMagicCode and
db.auth.signInWithMagicCode, and finally re-fetch the same $users row and assert
its extraFields still equal the original values (and that res2.created is false)
to ensure existing users are not updated.
In `@client/packages/core/vitest.config.ts`:
- Around line 4-5: The code uses Number(process.env.DEV_SLOT ?? 0) which yields
NaN for non-numeric DEV_SLOT values; update the logic in vitest.config.ts around
devSlot and localPort to validate DEV_SLOT explicitly: read the raw string from
process.env.DEV_SLOT, attempt to parse it (e.g., parseInt/Number), check
isFinite/Number.isNaN, and if invalid fall back to 0 (or optionally throw) and
compute localPort from the validated devSlot so stray non-numeric environment
values no longer produce NaN silently.
In `@client/packages/create-instant-app/template/rules/cursor-rules.md`:
- Around line 286-308: The example uses an undefined local `tx`
(tx.settings[id()]) causing inconsistency; update the snippet to reference the
full transaction helper `db.tx` (e.g., use db.tx.settings[id()]) or explicitly
show how `tx` is defined before use; ensure the example uses the same convention
as other examples with signInWithMagicCode/extraFields and the created flag so
the db.transact call references db.tx.settings[id()] (or introduces a prior
const tx = db.tx) for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 18a896ff-f102-4210-94a3-918ee8920a53
📒 Files selected for processing (23)
client/packages/admin/src/index.tsclient/packages/core/__tests__/src/auth-extra-fields.e2e.test.tsclient/packages/core/__tests__/src/utils/e2e.tsclient/packages/core/package.jsonclient/packages/core/src/Reactor.jsclient/packages/core/src/authAPI.tsclient/packages/core/src/index.tsclient/packages/core/vitest.config.tsclient/packages/create-instant-app/template/rules/AGENTS.mdclient/packages/create-instant-app/template/rules/cursor-rules.mdclient/packages/create-instant-app/template/rules/windsurf-rules.mdclient/sandbox/react-nextjs/pages/play/oauth-extra-fields-google.tsxclient/sandbox/react-nextjs/pages/play/oauth-extra-fields.tsxclient/www/lib/intern/instant-rules.mdclient/www/pages/docs/auth.mdclient/www/pages/docs/backend.mdclient/www/pages/docs/http-api.mdclient/www/pages/docs/users.mdserver/src/instant/admin/routes.cljserver/src/instant/model/app_user.cljserver/src/instant/runtime/magic_code_auth.cljserver/src/instant/runtime/routes.cljserver/test/instant/runtime/routes_test.clj
client/www/pages/docs/backend.md
Outdated
| const { user, created } = await db.auth.verifyMagicCode( | ||
| req.body.email, | ||
| req.body.code, | ||
| { extraFields: { nickname: req.body.nickname } }, | ||
| ); | ||
| const token = user.refresh_token; |
There was a problem hiding this comment.
Fix the Admin SDK verifyMagicCode example shape.
This page documents @instantdb/admin, but the snippet destructures { user, created } as if it were the client SDK response. The later example already shows the actual Admin SDK shape via user.created.
Suggested fix
-const { user, created } = await db.auth.verifyMagicCode(
+const user = await db.auth.verifyMagicCode(
req.body.email,
req.body.code,
{ extraFields: { nickname: req.body.nickname } },
);
+const { created } = user;
const token = user.refresh_token;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { user, created } = await db.auth.verifyMagicCode( | |
| req.body.email, | |
| req.body.code, | |
| { extraFields: { nickname: req.body.nickname } }, | |
| ); | |
| const token = user.refresh_token; | |
| const user = await db.auth.verifyMagicCode( | |
| req.body.email, | |
| req.body.code, | |
| { extraFields: { nickname: req.body.nickname } }, | |
| ); | |
| const { created } = user; | |
| const token = user.refresh_token; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/www/pages/docs/backend.md` around lines 403 - 408, The snippet
incorrectly destructures { user, created } from verifyMagicCode as if using the
client SDK; in the Admin SDK verifyMagicCode returns the user object (with
user.created), so change the call to capture the user directly (e.g., const user
= await db.auth.verifyMagicCode(...)) and then read token from
user.refresh_token and created from user.created; update any subsequent uses
(token and created) to reference the user object.
client/packages/core/src/Reactor.js
Outdated
| let extraFields; | ||
| if (typeof sessionStorage !== 'undefined') { | ||
| try { | ||
| const stored = sessionStorage.getItem(OAUTH_EXTRA_FIELDS_KEY); |
There was a problem hiding this comment.
What will happen in Expo?
--
imo it may make sense to store in the storage key that we have here, so we are good against different platforms. We may also want to use some specific key to signify this particular session
client/packages/admin/src/index.ts
Outdated
| }, | ||
| ); | ||
| return user; | ||
| return { ...res.user, created: res.created }; |
There was a problem hiding this comment.
What if someone has a field called created?
I think we may need to make a breaking change. Not sure if there's an alternative. cc @dwwoelfel for thoughts too
There was a problem hiding this comment.
Oof, yea what do we do here?
client/packages/core/src/Reactor.js
Outdated
| if (extraFields && typeof sessionStorage !== 'undefined') { | ||
| sessionStorage.setItem( | ||
| OAUTH_EXTRA_FIELDS_KEY, | ||
| JSON.stringify(extraFields), |
There was a problem hiding this comment.
Is there some way we could create a unique entry in the sessionStorage for each call to createAuthorizationUrl.
With the current implementation you can't remove the extra fields if you've set them and last call wins. An example of why you might want this: maybe you have two sign in urls, one sets source=homepage and one that sets source=pricing-page. With the current approach, you'd always get source=pricing-page if that was the last call to createAuthorizationUrl.
| (when (and (contains? params :imageURL) imageURL) | ||
| [[:add-triple id (resolve-id :imageURL) imageURL]]))) | ||
| [[:add-triple id (resolve-id :imageURL) imageURL]]) | ||
| (map (fn [[k v]] |
There was a problem hiding this comment.
Should we be running auth checks on these fields?
They're user-provided, so this might be surprising to apps that expected that their custom fields couldn't be set by the user.
There was a problem hiding this comment.
These should now be auth checked!
There was a problem hiding this comment.
♻️ Duplicate comments (1)
client/packages/core/src/Reactor.js (1)
2266-2273:⚠️ Potential issue | 🟠 MajorPrevent stale
extraFieldsreuse across OAuth attempts.
createAuthorizationURLonly writes whenextraFieldsexists, so a later flow withoutextraFieldscan reuse old persisted data. Also, one shared key makes concurrent auth links last-write-wins.Suggested minimum fix
createAuthorizationURL({ clientName, redirectURL, extraFields }) { - if (extraFields) { - this.kv.updateInPlace((prev) => { - prev[oauthExtraFieldsKey] = extraFields; - }); - } + this.kv.updateInPlace((prev) => { + if (extraFields) { + prev[oauthExtraFieldsKey] = extraFields; + } else { + delete prev[oauthExtraFieldsKey]; + } + }); const { apiURI, appId } = this.config; return `${apiURI}/runtime/oauth/start?app_id=${appId}&client_name=${clientName}&redirect_uri=${redirectURL}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/core/src/Reactor.js` around lines 2266 - 2273, createAuthorizationURL currently only writes extraFields to kv when present, causing stale data reuse and last-write-wins across concurrent flows; modify createAuthorizationURL to (1) when extraFields is provided, generate a unique per-attempt key (e.g., include a nonce/timestamp or clientName) instead of a single shared oauthExtraFieldsKey, persist extraFields under that unique key via kv.updateInPlace (or equivalent), and include that unique key as a query param in the returned URL; and (2) when extraFields is not provided, ensure you do not reuse an existing shared key—either omit the extraFields query param or explicitly clear the stored value for that unique key so no stale data is applied; reference createAuthorizationURL, kv.updateInPlace, and oauthExtraFieldsKey to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@client/packages/core/src/Reactor.js`:
- Around line 2266-2273: createAuthorizationURL currently only writes
extraFields to kv when present, causing stale data reuse and last-write-wins
across concurrent flows; modify createAuthorizationURL to (1) when extraFields
is provided, generate a unique per-attempt key (e.g., include a nonce/timestamp
or clientName) instead of a single shared oauthExtraFieldsKey, persist
extraFields under that unique key via kv.updateInPlace (or equivalent), and
include that unique key as a query param in the returned URL; and (2) when
extraFields is not provided, ensure you do not reuse an existing shared
key—either omit the extraFields query param or explicitly clear the stored value
for that unique key so no stale data is applied; reference
createAuthorizationURL, kv.updateInPlace, and oauthExtraFieldsKey to locate and
update the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 504a9d62-0422-43a4-ad0f-1991d3216819
📒 Files selected for processing (1)
client/packages/core/src/Reactor.js
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/src/instant/runtime/routes.clj (1)
583-600:⚠️ Potential issue | 🟠 MajorRun request validation before consuming the OAuth code.
Line 587 invalidates the one-time code before the request-only checks below (
Originandextra_fields) run. With the newextra_fieldssupport, a typo now forces the user back through the provider flow even though the grant itself was valid.
↕️ Suggested reordering(let [app-id (ex/get-some-param! req (param-paths :app_id) uuid-util/coerce) code (ex/get-some-param! req (param-paths :code) uuid-util/coerce) code-verifier (some #(get-in req %) (param-paths :code_verifier)) extra-fields (get-in req [:body :extra_fields]) - oauth-code (app-oauth-code-model/consume! {:code code - :app-id app-id - :verifier code-verifier}) _ (when-let [origin (get-in req [:headers "origin"])] (let [authorized-origins (app-authorized-redirect-origin-model/get-all-for-app {:app-id app-id})] (when-not (app-authorized-redirect-origin-model/find-match authorized-origins origin) (ex/throw-validation-err! :origin origin [{:message "Unauthorized origin."}])))) _ (app-user-model/validate-extra-fields! app-id extra-fields) + oauth-code (app-oauth-code-model/consume! {:code code + :app-id app-id + :verifier code-verifier})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/runtime/routes.clj` around lines 583 - 600, Reorder the request handling so validation runs before the one-time OAuth code is consumed: keep computing app-id, code, code-verifier, and extra-fields from req as-is, then perform the Origin header check (using app-authorized-redirect-origin-model/get-all-for-app and find-match) and call app-user-model/validate-extra-fields! before calling app-oauth-code-model/consume!; after consume! you can destructure {:keys [app_id client_id user_info]} from oauth-code and assert (= app-id app_id) as before. Ensure the call site of app-oauth-code-model/consume! (and the subsequent equality assert) is moved below the two validation steps so a validation error does not prematurely invalidate the one-time code.server/src/instant/runtime/magic_code_auth.clj (1)
141-160:⚠️ Potential issue | 🟠 MajorDon't merge the
createdflag into the flat user map.
validate-extra-fields!only rejects unknown/system attrs, so a schema-defined$users.createdis still legal. Theassocon Line 160 overwrites that user field for both new and existing users, andverify-magic-code-postthen strips it back out in Lines 110-111 ofserver/src/instant/runtime/routes.clj, so a legitimate attribute disappears from magic-code sign-in responses.Return
createdalongside the user instead of merging it into the entity map, or reservecreatedduring extra-field validation.
♻️ Duplicate comments (3)
client/sandbox/react-nextjs/pages/play/oauth-extra-fields-google.tsx (1)
101-105:⚠️ Potential issue | 🟠 MajorMove redirect URL creation out of render.
window.location.hrefin render can break SSR, and callingcreateAuthorizationURL()during render can repeatedly execute redirect-persistence side effects. Compute it in the click handler instead.🛠️ Suggested fix
- const redirectLoginURL = db.auth.createAuthorizationURL({ - clientName: 'google-web', - redirectURL: window.location.href, - extraFields: displayName ? { displayName } : undefined, - }); - return ( @@ - <a - href={redirectLoginURL} - className="inline-block rounded bg-blue-500 px-4 py-2 text-white" + <button + type="button" + className="inline-block rounded bg-blue-500 px-4 py-2 text-white" + onClick={() => { + const url = db.auth.createAuthorizationURL({ + clientName: 'google-web', + redirectURL: window.location.href, + extraFields: displayName ? { displayName } : undefined, + }); + window.location.assign(url); + }} > Google (Redirect) - </a> + </button>#!/bin/bash # Verify render-time usage of window/createAuthorizationURL on this page. rg -n "createAuthorizationURL|window\\.location\\.href|href=\\{redirectLoginURL\\}" client/sandbox/react-nextjs/pages/play/oauth-extra-fields-google.tsxAlso applies to: 125-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/sandbox/react-nextjs/pages/play/oauth-extra-fields-google.tsx` around lines 101 - 105, The code currently computes redirectLoginURL during render using window.location.href and db.auth.createAuthorizationURL (variable redirectLoginURL), which breaks SSR and causes side effects; move the createAuthorizationURL call into the sign-in click handler (the onClick that triggers the login flow), compute const redirectURL = window.location.href inside that handler (or derive it safely client-side) and then call db.auth.createAuthorizationURL({ clientName: 'google-web', redirectURL, extraFields: displayName ? { displayName } : undefined }) there so nothing runs at render time and the extraFields (displayName) are passed from the current component state.client/www/pages/docs/users.md (1)
237-243:⚠️ Potential issue | 🟡 MinorUse
db.txin this docs snippet.Line 240 references
tx.settings, buttxis not defined anywhere in the example. As written, the copy-paste path throws before it creates the default record.📝 Suggested fix
if (created) { // Create default data for the new user db.transact([ - tx.settings[id()] + db.tx.settings[id()] .update({ theme: 'light', notifications: true }) .link({ user: user.id }), ]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/www/pages/docs/users.md` around lines 237 - 243, The snippet uses an undefined identifier `tx` inside the db.transact call (tx.settings[id()].update(...).link(...)), causing a runtime error; replace the `tx` usage with the correct transactional API `db.tx` (i.e., use db.tx.settings[id()].update(...) .link(...)) so the default user settings are created within the transaction invoked by db.transact/db.tx and the references to created, db.transact and settings[id()].update(...).link(...) remain consistent.client/packages/core/src/Reactor.js (1)
2282-2290:⚠️ Potential issue | 🟠 MajorClear stale OAuth
extraFieldswhen a new flow doesn't provide any.The current implementation only writes to storage when
extraFieldsis provided. If a previous OAuth flow stored fields and the next attempt omits them, the old payload survives and_oauthLoginInit()will attach it to the later sign-in.Suggested fix
createAuthorizationURL({ clientName, redirectURL, extraFields }) { - if (extraFields) { - this.kv.updateInPlace((prev) => { - prev[oauthExtraFieldsKey] = extraFields; - }); - } + this.kv.updateInPlace((prev) => { + if (extraFields) { + prev[oauthExtraFieldsKey] = extraFields; + } else { + delete prev[oauthExtraFieldsKey]; + } + }); const { apiURI, appId } = this.config; return `${apiURI}/runtime/oauth/start?app_id=${appId}&client_name=${clientName}&redirect_uri=${redirectURL}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/core/src/Reactor.js` around lines 2282 - 2290, The createAuthorizationURL function only updates storage when extraFields is present, leaving stale oauthExtraFieldsKey values around; modify createAuthorizationURL so that when extraFields is falsy it explicitly clears the stored value (e.g., call this.kv.updateInPlace and delete or null-out prev[oauthExtraFieldsKey]) so subsequent calls (and _oauthLoginInit) won't pick up prior extraFields; keep the existing write behavior when extraFields is provided.
🧹 Nitpick comments (2)
client/sandbox/react-nextjs/pages/play/oauth-extra-fields.tsx (1)
136-137: Consider removing temporary debug logs before merge.These
console.logcalls are useful during development, but trimming them keeps sandbox output cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/sandbox/react-nextjs/pages/play/oauth-extra-fields.tsx` around lines 136 - 137, Remove the temporary console.log debug statements in the signInWithMagicCode flow (the lines logging 'signInWithMagicCode result:' and 'created:') — either delete them or replace with a proper debug/logger call if persistent telemetry is desired (e.g., use the app's logger at debug level). Ensure you update the function handling sign-in so no leftover console.log remains after verification.client/packages/create-instant-app/template/rules/windsurf-rules.md (1)
294-298: Avoid using a client clock forcreatedAtin the signup example.Line 297 makes
createdAtlook like a trusted signup timestamp, but this rules file is meant to be copied into generated apps andDate.now()here is entirely client-controlled. A neutral profile field is safer unless the timestamp is set server-side.✏️ Suggested tweak
const { user, created } = await db.auth.signInWithMagicCode({ email, code, - extraFields: { nickname, createdAt: Date.now() }, + extraFields: { nickname }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/create-instant-app/template/rules/windsurf-rules.md` around lines 294 - 298, The example uses Date.now() for the signup `createdAt` in the call to db.auth.signInWithMagicCode (inside extraFields), which relies on an untrusted client clock; update the example to avoid setting a client-controlled timestamp—remove or null out the `createdAt` field (or replace it with a neutral profile field like `bio` or `signupSource`) in the `extraFields` passed to db.auth.signInWithMagicCode so that the server can set the canonical creation timestamp instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/packages/core/__tests__/src/auth-extra-fields.e2e.test.ts`:
- Around line 20-31: The helper currently returns data.code without checking the
HTTP response, so failures are masked; update the helper that calls fetch (the
block using apiUrl/admin/magic_code with appId and adminToken) to check res.ok
and throw a descriptive error when not OK (include res.status and the response
body or JSON) before returning data.code, so callers get a clear failure instead
of undefined.
In `@client/www/pages/docs/http-api.md`:
- Around line 270-272: The prose incorrectly says "provider id or email" which
conflicts with the documented contract for POST /admin/refresh_tokens; update
the copy to refer to the endpoint's accepted inputs as "id or email" (or
explicitly "user id or email") so it matches the endpoint contract, keep the
rest of the sentence about passing `extra-fields` to set `$users` properties and
that the response includes `user.refresh_token` and `"created": true` when a new
user is created, and optionally add a short link reference to the existing POST
/admin/refresh_tokens section for clarity.
In `@server/src/instant/admin/routes.clj`:
- Around line 400-402: The extra-fields validation (the call to
app-user-model/validate-extra-fields! using extra-fields from req) and the
magic-code verification should be deferred from the shared pre-check path into
the branch that handles creating a new user; update routes.clj so you first
determine if the user already exists (the logic used in the
refresh-tokens/sign-in path) and only call app-user-model/validate-extra-fields!
and the magic-code validation in the create/new-user branch (and likewise move
the validations out of the refresh-token/pre-auth path), ensuring existing-user
sign-ins skip those validations.
In `@server/src/instant/model/app_user.clj`:
- Around line 15-33: validate-extra-fields! must reject the reserved key
"created" to avoid response collisions; inside the existing loop (in
validate-extra-fields! which calls attr-model/get-by-app-id and
attr-model/seek-by-fwd-ident-name) add a check after computing k-str and before
attr validation that if k-str equals "created" then call
ex/throw-validation-err! with :extra-fields, extra-fields and a message like
"Cannot set reserved field: created" so user-supplied $users.created is
rejected.
---
Outside diff comments:
In `@server/src/instant/runtime/routes.clj`:
- Around line 583-600: Reorder the request handling so validation runs before
the one-time OAuth code is consumed: keep computing app-id, code, code-verifier,
and extra-fields from req as-is, then perform the Origin header check (using
app-authorized-redirect-origin-model/get-all-for-app and find-match) and call
app-user-model/validate-extra-fields! before calling
app-oauth-code-model/consume!; after consume! you can destructure {:keys [app_id
client_id user_info]} from oauth-code and assert (= app-id app_id) as before.
Ensure the call site of app-oauth-code-model/consume! (and the subsequent
equality assert) is moved below the two validation steps so a validation error
does not prematurely invalidate the one-time code.
---
Duplicate comments:
In `@client/packages/core/src/Reactor.js`:
- Around line 2282-2290: The createAuthorizationURL function only updates
storage when extraFields is present, leaving stale oauthExtraFieldsKey values
around; modify createAuthorizationURL so that when extraFields is falsy it
explicitly clears the stored value (e.g., call this.kv.updateInPlace and delete
or null-out prev[oauthExtraFieldsKey]) so subsequent calls (and _oauthLoginInit)
won't pick up prior extraFields; keep the existing write behavior when
extraFields is provided.
In `@client/sandbox/react-nextjs/pages/play/oauth-extra-fields-google.tsx`:
- Around line 101-105: The code currently computes redirectLoginURL during
render using window.location.href and db.auth.createAuthorizationURL (variable
redirectLoginURL), which breaks SSR and causes side effects; move the
createAuthorizationURL call into the sign-in click handler (the onClick that
triggers the login flow), compute const redirectURL = window.location.href
inside that handler (or derive it safely client-side) and then call
db.auth.createAuthorizationURL({ clientName: 'google-web', redirectURL,
extraFields: displayName ? { displayName } : undefined }) there so nothing runs
at render time and the extraFields (displayName) are passed from the current
component state.
In `@client/www/pages/docs/users.md`:
- Around line 237-243: The snippet uses an undefined identifier `tx` inside the
db.transact call (tx.settings[id()].update(...).link(...)), causing a runtime
error; replace the `tx` usage with the correct transactional API `db.tx` (i.e.,
use db.tx.settings[id()].update(...) .link(...)) so the default user settings
are created within the transaction invoked by db.transact/db.tx and the
references to created, db.transact and settings[id()].update(...).link(...)
remain consistent.
---
Nitpick comments:
In `@client/packages/create-instant-app/template/rules/windsurf-rules.md`:
- Around line 294-298: The example uses Date.now() for the signup `createdAt` in
the call to db.auth.signInWithMagicCode (inside extraFields), which relies on an
untrusted client clock; update the example to avoid setting a client-controlled
timestamp—remove or null out the `createdAt` field (or replace it with a neutral
profile field like `bio` or `signupSource`) in the `extraFields` passed to
db.auth.signInWithMagicCode so that the server can set the canonical creation
timestamp instead.
In `@client/sandbox/react-nextjs/pages/play/oauth-extra-fields.tsx`:
- Around line 136-137: Remove the temporary console.log debug statements in the
signInWithMagicCode flow (the lines logging 'signInWithMagicCode result:' and
'created:') — either delete them or replace with a proper debug/logger call if
persistent telemetry is desired (e.g., use the app's logger at debug level).
Ensure you update the function handling sign-in so no leftover console.log
remains after verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d88e4f2c-4540-4208-9bca-87640d5afda6
📒 Files selected for processing (23)
client/packages/admin/src/index.tsclient/packages/core/__tests__/src/auth-extra-fields.e2e.test.tsclient/packages/core/__tests__/src/utils/e2e.tsclient/packages/core/package.jsonclient/packages/core/src/Reactor.jsclient/packages/core/src/authAPI.tsclient/packages/core/src/index.tsclient/packages/core/vitest.config.tsclient/packages/create-instant-app/template/rules/AGENTS.mdclient/packages/create-instant-app/template/rules/cursor-rules.mdclient/packages/create-instant-app/template/rules/windsurf-rules.mdclient/sandbox/react-nextjs/pages/play/oauth-extra-fields-google.tsxclient/sandbox/react-nextjs/pages/play/oauth-extra-fields.tsxclient/www/lib/intern/instant-rules.mdclient/www/pages/docs/auth.mdclient/www/pages/docs/backend.mdclient/www/pages/docs/http-api.mdclient/www/pages/docs/users.mdserver/src/instant/admin/routes.cljserver/src/instant/model/app_user.cljserver/src/instant/runtime/magic_code_auth.cljserver/src/instant/runtime/routes.cljserver/test/instant/runtime/routes_test.clj
🚧 Files skipped from review as they are similar to previous changes (6)
- client/packages/admin/src/index.ts
- client/packages/core/vitest.config.ts
- client/www/pages/docs/backend.md
- client/packages/core/src/index.ts
- client/packages/create-instant-app/template/rules/AGENTS.md
- client/packages/create-instant-app/template/rules/cursor-rules.md
| const res = await fetch(`${apiUrl}/admin/magic_code`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'app-id': appId, | ||
| authorization: `Bearer ${adminToken}`, | ||
| }, | ||
| body: JSON.stringify({ email }), | ||
| }); | ||
| const data = await res.json(); | ||
| return data.code; | ||
| } |
There was a problem hiding this comment.
Fail fast when magic-code generation returns non-2xx.
The helper returns data.code without checking res.ok, which can mask the real failure source.
🛠️ Suggested fix
const res = await fetch(`${apiUrl}/admin/magic_code`, {
@@
});
+ if (!res.ok) {
+ throw new Error(await res.text());
+ }
const data = await res.json();
+ if (!data?.code) {
+ throw new Error('Missing magic code in admin/magic_code response');
+ }
return data.code;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/packages/core/__tests__/src/auth-extra-fields.e2e.test.ts` around
lines 20 - 31, The helper currently returns data.code without checking the HTTP
response, so failures are masked; update the helper that calls fetch (the block
using apiUrl/admin/magic_code with appId and adminToken) to check res.ok and
throw a descriptive error when not OK (include res.status and the response body
or JSON) before returning data.code, so callers get a clear failure instead of
undefined.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/test/instant/runtime/routes_test.clj (2)
647-672: Remove unusedappbinding.The static analyzer flagged an unused
appbinding at line 649. The test only usesapp-id.🧹 Suggested fix
(deftest users-create-rule-oauth-test (with-empty-app - (fn [{app-id :id :as app}] + (fn [{app-id :id}]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/instant/runtime/routes_test.clj` around lines 647 - 672, The test binding includes an unused :as app in the with-empty-app callback; update the argument destructuring in users-create-rule-oauth-test from (fn [{app-id :id :as app}]) to only capture app-id (e.g. (fn [{app-id :id}]) ) so the unused app symbol is removed, leaving the rest of the test (rule-model/put!, route/upsert-oauth-link!, assertions) unchanged.
486-518: Remove unusedappbinding.The static analyzer flagged an unused
appbinding at line 488. The test only usesapp-id.🧹 Suggested fix
(deftest extra-fields-oauth-test (with-empty-app - (fn [{app-id :id :as app}] + (fn [{app-id :id}]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/instant/runtime/routes_test.clj` around lines 486 - 518, The anonymous function in extra-fields-oauth-test currently destructures its argument as ({app-id :id :as app}) but never uses the app binding; remove the unused :as app to silence the analyzer by changing the parameter destructuring to ({app-id :id}) in the with-empty-app callback (the anonymous fn used inside extra-fields-oauth-test) so only app-id is bound.server/src/instant/db/permissioned_transaction.clj (1)
49-57: Remove unusedeidbinding.The
eidparameter is destructured but never used in the function body. The static analyzer correctly flagged this.🧹 Suggested fix
-(defn- validate-system-create-entity! [{:keys [admin? attrs]} {:keys [aid eid] :as tx-step}] +(defn- validate-system-create-entity! [{:keys [admin? attrs]} {:keys [aid] :as tx-step}] (let [attr (attr-model/seek-by-id aid attrs) [etype label] (attr-model/fwd-ident-name attr)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/permissioned_transaction.clj` around lines 49 - 57, The destructured eid in the function validate-system-create-entity! is unused; remove it from the destructuring vector (change {:keys [aid eid] :as tx-step} to {:keys [aid] :as tx-step}) or rename it to _eid to silence the analyzer, updating only the parameter list in validate-system-create-entity! so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/www/pages/docs/users.md`:
- Around line 237-244: The code example uses an unimported bare symbol tx;
replace usages of tx inside the transaction array with db.tx so the example is
consistent with other docs (e.g., change tx.settings[id()] to
db.tx.settings[id()]) and ensure the db.transact call remains the same so the
transaction uses the namespaced transaction builder provided by db.
---
Nitpick comments:
In `@server/src/instant/db/permissioned_transaction.clj`:
- Around line 49-57: The destructured eid in the function
validate-system-create-entity! is unused; remove it from the destructuring
vector (change {:keys [aid eid] :as tx-step} to {:keys [aid] :as tx-step}) or
rename it to _eid to silence the analyzer, updating only the parameter list in
validate-system-create-entity! so behavior remains unchanged.
In `@server/test/instant/runtime/routes_test.clj`:
- Around line 647-672: The test binding includes an unused :as app in the
with-empty-app callback; update the argument destructuring in
users-create-rule-oauth-test from (fn [{app-id :id :as app}]) to only capture
app-id (e.g. (fn [{app-id :id}]) ) so the unused app symbol is removed, leaving
the rest of the test (rule-model/put!, route/upsert-oauth-link!, assertions)
unchanged.
- Around line 486-518: The anonymous function in extra-fields-oauth-test
currently destructures its argument as ({app-id :id :as app}) but never uses the
app binding; remove the unused :as app to silence the analyzer by changing the
parameter destructuring to ({app-id :id}) in the with-empty-app callback (the
anonymous fn used inside extra-fields-oauth-test) so only app-id is bound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c39d75a-2754-4a51-8164-e9f96be02124
📒 Files selected for processing (13)
client/packages/create-instant-app/template/rules/AGENTS.mdclient/packages/create-instant-app/template/rules/cursor-rules.mdclient/packages/create-instant-app/template/rules/windsurf-rules.mdclient/sandbox/react-nextjs/pages/play/signup-rules.tsxclient/www/lib/intern/instant-rules.mdclient/www/pages/docs/users.mdserver/src/instant/admin/routes.cljserver/src/instant/db/permissioned_transaction.cljserver/src/instant/model/app_user.cljserver/src/instant/model/rule.cljserver/src/instant/runtime/magic_code_auth.cljserver/src/instant/runtime/routes.cljserver/test/instant/runtime/routes_test.clj
✅ Files skipped from review due to trivial changes (1)
- client/packages/create-instant-app/template/rules/cursor-rules.md
🚧 Files skipped from review as they are similar to previous changes (4)
- client/packages/create-instant-app/template/rules/windsurf-rules.md
- server/src/instant/runtime/routes.clj
- server/src/instant/admin/routes.clj
- client/packages/create-instant-app/template/rules/AGENTS.md
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When someone signs up with Instant, we create a bare user with just an email. If a dev wants to add custom fields, they have to run a separate transaction after login. There's also no way to distinguish a new signup from a returning user, and no way to restrict who can sign up.
Solution
extraFields+createdEvery auth flow now accepts an optional
extraFieldsmap and returns acreatedboolean.extraFieldswrites custom$usersproperties atomically during user creation. If the user already exists, the fields are ignored and validation is skipped. Returningcreatedhelps clients differentiate between sign-up and login.For web redirects,
extraFieldsis stored in the kv store under a unique ID that's appended to the redirect URL. EachcreateAuthorizationURLcall gets its own ID so multiple sign-in links on the same page don't overwrite each other. Entries are cleaned up after login.checkMagicCode(admin SDK)The existing
verifyMagicCodein the admin SDK returns aUserdirectly. Addingcreatedto it would risk colliding with a user-definedcreatedfield. So we addedcheckMagicCodewhich returns{ user, created }and supports typedextraFieldsvia the schema.verifyMagicCodeis deprecated but unchanged for backwards compatibility.$userscreate permission rulesDevs can now write
createrules on$usersto restrict who can sign up or validateextraFields. The rule runs during auth signup flows only (not viadb.transact, which is blocked by a newvalidate-system-create-entity!check in permissioned transactions).Default is
true(anyone can sign up).authanddataare both set to the user being created.ref()is not available since no relationships exist yet.In the magic code flow, the permission check runs before consuming the code so a failed check doesn't burn the one-time code.
Tests
Server tests (
routes_test.clj)extraFields tests:
created: true, returning user ignores fields +created: false, backwards compat, unknown keys rejected, system fields rejected, returning user with invalid extraFields still signs inupsert-oauth-link!direct call, new + existing userrefresh_tokens: create applies extra-fields, existing user ignoresCreate permission tests:
$userscreation viadb.transactblocked even with create rule set to trueRule model test (
rule_test.clj):$usersE2e tests (separate PR #2394)
Moved to
extra-fields-e2e-authbranch since they hit prod which doesn't have the backend changes yet.Manual sandbox tests
/play/oauth-extra-fields-- magic code flow with extraFields on an ephemeral app/play/oauth-extra-fields-google-- Google OAuth with extraFields/play/signup-rules-- interactive testing of$userscreate permission rules with domain restriction and field validationDocs + Rules
users.md-- "Setting properties at signup" section with examples for all auth patterns pluscreated-based scaffolding. New "Signup rules" section with examples for domain restriction, field validation, and waitlist mode.backend.md--checkMagicCodeexample with extraFields and created.http-api.md--extra-fieldsdocumented onrefresh_tokensandverify_magic_codeendpoints.instant-rules.md-- Updated$userspermissions to reflect create rules are now supported.